-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finalize the latest epoch #1082
Finalize the latest epoch #1082
Conversation
This comment has been minimized.
This comment has been minimized.
src/test/esperanza/finalizationstate_calculate_withdraw_amount_tests.cpp
Show resolved
Hide resolved
m_reward_factor = 0; | ||
} | ||
|
||
IncrementDynasty(); | ||
|
||
if (GetActiveFinalizers().empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I changed DepositExists()
to GetActiveFinalizers().empty()
is because DepositExists()
has a bit different meaning. It checks that we have deposits in current and previous dynasty. However, GetActiveFinalizers
checks that there is a finalizer in current or previous dynasty. If finalizer has deposited in one of the dynasties, it can vote and participate in finalization. So, if we kept DepositExists()
we have "race-condition" were at the same to systems are in place, instant justification and votes. By changing this, we have always 1 mechanism, either instant justification or voting.
@@ -199,7 +199,7 @@ ufp64::ufp64_t FinalizationState::GetCollectiveRewardFactor() { | |||
} | |||
|
|||
bool FinalizationState::DepositExists() const { | |||
return m_cur_dyn_deposits > 0; | |||
return m_cur_dyn_deposits > 0 && m_prev_dyn_deposits > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is the revert from a0267c3
I misunderstood the meaning of this function as we didn't have tests for reward (I could adjust the code that the whole deposit would be burnt and all tests would pass). But now there is a test that covers finalizer reward so I return this block back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConceptACK. I think this proposal moves us closer to initial Casper idea.
I went through the changes, most of them are monotonous one-lines that replaces numbers, so I could miss something.
m_last_finalized_epoch = expected_finalized; | ||
uint32_t prev_justified = m_current_epoch - 2; | ||
if (GetCheckpoint(prev_justified).m_is_justified) { | ||
cp.m_is_finalized = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't GetCheckpoint(prev_justified)
become finalized as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the change proposed here: https://github.com/dtr-org/unit-e/pull/1083/files#diff-924113d2ba8b7131c0af6bb2fe3c1d75R1088 it's finalized, but I suggest keeping data consistent as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frolosofsky I think it's OK to keep previous epoch only justified. It's nice that we can look back and see if we reached finalization at that point or not. Otherwise, after we reached finalization, we should go through all checkpoints until the current one and mark them finalized, but I don't think it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frolosofsky I think it's OK to keep previous epoch only justified. It's nice that we can look back and see if we reached finalization at that point or not. Otherwise, after we reached finalization, we should go through all checkpoints until the current one and mark them finalized, but I don't think it's needed.
I don't think so, honestly. You've already rewritten comments about justified->finalized because they were wrong, and why not change that in the code? Having #1083 merged will strictly limit amount of checkpoints.
(nit and up to you, I understand that this code works)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment wasn't wrong but I changed as it seemed confusing.
Eventually, I'll delete all checkpoints, but since it's harder to do, we have them. My point is that every FinalizationState (or checkpoints while we still have them) contains the information which was valid/up to date at the time this state was created/(considered the current one), and we shouldn't override old data (previous states) when we started to create new ones. Currently, this is true (I spot only 1 case when this rule is violated in FinalizationState but will fix it separately).
I believe that relying on this immutability in this particular case will give us a safer code as it easier to maintain/less risky to unsynchronize. For instance, let's say we made old checkpoints is_finalized=true
but then if we retrieve FinalizationState
for these checkpoints, they won't have is_finalized=true
flag.
e707890
to
0f891e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5f330a7
Concept ACK 5f330a7 |
This comment has been minimized.
This comment has been minimized.
When we have two consecutive justifications, finalize the latest one instead of the previous one. Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
5f330a7
to
aa7a2f7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
src/test/esperanza/finalizationstate_calculate_withdraw_amount_tests.cpp
Outdated
Show resolved
Hide resolved
if (GetCheckpoint(expected_finalized).m_is_justified) { | ||
GetCheckpoint(expected_finalized).m_is_finalized = true; | ||
m_last_finalized_epoch = expected_finalized; | ||
uint32_t to_be_finalized = m_current_epoch - 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a bit misleading that this variable is named to_be_finalized
but we finalize the next epoch m_current_epoch - 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name was changed from prev_justified
to this after discussing in the following thread #1082 (comment)
I am fine to change it again if we have a consensus :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gnappuraz's comment doesn't actually make sense to me. prev_justified
seems to be closer to the purpose of this variable.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 47edbaa
ConceptACK 47edbaa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 47edbaa
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Signed-off-by: Kostiantyn Stepaniuk <[email protected]>
Did a quick review, it looks ok, utACK |
When we have two consecutive justifications, finalize the latest one instead of the previous one.
Rationale
Let's say we have these two consecutive justified links.
In Casper the finalized epoch would be e4 and in our case it's e3. However, we proved in feature_fork_choice_forked_finalize_epoch.py that reorging e4, in this case, is not possible anymore as it will revert finalization for e3. Because of that we introduced
FinalizationState::GetCheckpointHeightAfterFinalizedEpoch
as a point of "not going back". However, if we count e4 as finalized, we can drop this extra entity. Moreover, we used to have such a convention and we changed but as it turned out wasn't a good idea.Aditional fix
CalculateWithdrawAmount
was broken as it didn't correctly take reward into account which led to a shrunk amount over time. We didn't have tests to catch it. Now we havefinalizationstate_calculate_withdraw_amount_tests.cpp
test that tests this part and shows that the amount is properly increasing.However the
finalization_withdraw.py
still shows that after withdraw the balance is not larger than the initial deposit (it's actually equal to initial deposit - fee). The reason is that the reward for finalizers is not implemented and I'll continue working on it in the following PR.Resolves #1092
Signed-off-by: Kostiantyn Stepaniuk [email protected]